-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SecureComms: Add testing facility for e2e tests #2124
base: main
Are you sure you want to change the base?
Conversation
9480971
to
d52fc4e
Compare
This PR is expected to fail when using the existing workflow and succeed when using the modified workflow. It fails now since the existing workflow calls config_libvirt.sh without a parameter As an alternative we could create a new script (e.g. config_libvirt_matrix.sh) that will return the list of matrix options. |
4f3837e
to
c5e6e99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking cool - thanks for adding the matrix json, this could be a really good pattern assuming it works fine 😄
|
||
# switch to the appropriate e2e test and add configs to libvirt.properties as needed | ||
case $TEST_E2E_SECURE_COMMS in | ||
|
||
*) | ||
echo "processing none" | ||
;; | ||
esac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this debug here given that it is echo'd above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should stay here as it is the basis of what we will do next - e.g.:
# switch to the appropriate e2e test and add configs to libvirt.properties as needed
case $TEST_E2E_SECURE_COMMS in
"withoutKbs")
echo "processing withoutKbs"
echo "SECURE_COMMS=\"true\"" >> libvirt.properties
echo "SECURE_COMMS_KBS_ADDR=\"false\"" >> libvirt.properties
echo "INITDATA=\"\"" >> libvirt.properties
;;
*)
echo "processing none"
;;
esac
It maybe better to remove the earlier echo "SECURE_COMMS is ${TEST_E2E_SECURE_COMMS}"
if it feels we are putting too much output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you add that in when you start using it? At the moment it's not doing anything useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Myabe the right thing to do is to spell this out more clearly by adding echo "SECURE_COMMS=\"false\"" >> libvirt.properties
So we will have:
# switch to the appropriate e2e test and add configs to libvirt.properties as needed
case $TEST_E2E_SECURE_COMMS in
*)
echo "processing none"
echo "SECURE_COMMS=\"false\"" >> libvirt.properties
;;
esac
Will this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's okay, it won't be used by anything, so I still don't understand the need for it in this PR, but 🤷
c5e6e99
to
0c2fb6f
Compare
Modify the .github/workflow files to enable addting e2e tests for secure comms. Signed-off-by: David Hadas <[email protected]> Signed-off-by: David Hadas <[email protected]>
0c2fb6f
to
74103f1
Compare
Modify the .github/workflow files to enable adding e2e tests for secure comms.
The added code is generalized to enable controlling the libvirt e2e testing matrix using a json file